Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Resolve pytype --strict-none-binding issue in the api client #3214

Merged
merged 9 commits into from
Nov 27, 2024

Conversation

jkppr
Copy link
Collaborator

@jkppr jkppr commented Oct 21, 2024

This PR addresses pytype errors raised by the --strict-none-binding flag in the api client Search class. The issue stemmed from self._raw_response being initialized to None and then populated later by _execute_query, leading to potential unsoundness when accessing it directly.

The fix modifies _execute_query to always return a value: a dictionary with the search results if the results are not saved to a file and count=False, the total count if count=True, or None if the results are saved to a file. The to_dict method now calls _execute_query if self._raw_response is None and handles the case where _execute_query returns None (indicating a previous to_file call) by raising a ValueError. This approach avoids the less robust solution of using assertions and ensures that to_pandas and other methods always have a valid dictionary to work with.

A similar issue was fixed in story.py to handle cases where a view block did not have a populated view attribute, preventing a potential error.

These changes improve type safety and robustness in the Search and Story classes.

@jkppr jkppr requested a review from tomchop October 21, 2024 16:10
@jkppr jkppr self-assigned this Oct 21, 2024
@jkppr jkppr requested a review from tomchop November 27, 2024 16:42
@jkppr jkppr merged commit c49ec7e into google:master Nov 27, 2024
24 checks passed
@jkppr jkppr deleted the pytype-strict-none-binding branch November 27, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants